Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve nvda helper by compiling with C++20 and disabling the /permissive flag #13072

Merged
merged 21 commits into from Oct 24, 2022
Merged

Improve nvda helper by compiling with C++20 and disabling the /permissive flag #13072

merged 21 commits into from Oct 24, 2022

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Nov 19, 2021 •
edited by feerrenrut

Link to issue number:

None

Summary of the issue:

NVDAHelper was compiled with C++17, it could be compiled with C++20

  1. Visual Studio has support for C++20.
  2. WinRT is C++20 (Convert NVDAHelper LocalWin10 to C++/WinRT #11768).
  3. Compiling with C++20 ensures we're more conformant.
    • Passing string literals to parameters expecting wchar_t* or BSTR is no longer supported. This ensures safe code. See here for a rationale.
    • The/permissive- flag (note the minus symbol disabling permissive mode), ensuring conformance in more areas. See here

Description of how this pull request fixes the issue:

  • Change build params so nvdaHelper is built with C++20 and disable permissive mode.
  • Fix errors (wchar_t* or BSTR from string literal)

Testing strategy:

The following areas of code have been touched and need testing:

  • UIA custom automation types (comments in Excel with UIA and Windows 11)
  • Adobe reader virtual buffers
  • Gecko virtual buffers
  • MSHTML virtual buffers
  • Webkit virtual buffers

Other changes are mainly changes from char* to const char*, so unlikely to cause any harm.

I did some quick tests and didn't find anything crucial, probably best to test this in Alpha.

Known issues with pull request:

None

Change log entries:

For Developers

  • NVDAHelper is now C++20 and built with the /permissive- compiler flag, which ensures better conformance to C++ standards.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@LeonarddeR LeonarddeR changed the title Improve nvda helper by compiling with C++17 and enabling the /permissive- flag Improve nvda helper by compiling with C++20 and enabling the /permissive- flag Nov 19, 2021
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@LeonarddeR

This comment was marked as outdated.

Sign in to view
@LeonarddeR LeonarddeR marked this pull request as ready for review November 26, 2021 18:43
@LeonarddeR LeonarddeR requested a review from a team as a code owner November 26, 2021 18:43
@AppVeyorBot

This comment was marked as resolved.

Sign in to view
@feerrenrut

This comment was marked as resolved.

Sign in to view
@feerrenrut feerrenrut marked this pull request as draft February 16, 2022 10:59
@LeonarddeR LeonarddeR marked this pull request as ready for review February 18, 2022 07:17
@LeonarddeR
Copy link
Collaborator Author

The build issue should have been fixed. It was due to a bad merge.

@feerrenrut
Copy link
Contributor

Unless there is some urgency on this that I'm not aware of, I think it would be better to merge this in the new dev cycle. Applying 2022.2 milestone.

@feerrenrut feerrenrut added this to the 2022.2 milestone Feb 21, 2022
@feerrenrut feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022
@seanbudd
Copy link
Member

We plan to merge this soon for 2022.2, and are ready to revert if there are issues

@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@LeonarddeR
Copy link
Collaborator Author

It looks like the UIA remote library doesn't like C++20 that much. I need to dive into this some more.

@LeonarddeR LeonarddeR marked this pull request as draft May 23, 2022 08:11
@LeonarddeR LeonarddeR changed the title Improve nvda helper by compiling with C++20 and enabling the /permissive- flag Improve nvda helper by compiling with C++20 and disabling the /permissive flag May 23, 2022
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@seanbudd seanbudd removed this from the 2022.2 milestone Jun 16, 2022
@seanbudd seanbudd requested review from feerrenrut and removed request for michaelDCurran June 17, 2022 05:14
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@feerrenrut

This comment was marked as resolved.

Sign in to view
@feerrenrut feerrenrut added the merge-early Merge Early in a developer cycle label Jul 27, 2022
@LeonarddeR LeonarddeR marked this pull request as ready for review October 15, 2022 09:08
@LeonarddeR
Copy link
Collaborator Author

This is now ready for review. Probably want to merge this early.

@feerrenrut feerrenrut merged commit d998806 into nvaccess:master Oct 24, 2022
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feerrenrut feerrenrut feerrenrut approved these changes

Assignees
No one assigned
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. merge-early Merge Early in a developer cycle
Projects
None yet
Milestone
2023.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@LeonarddeR @AppVeyorBot @feerrenrut @seanbudd @nvaccessAuto